Repository navigation
Fix PropagateCastOps when a removed Cast feeds the same node input twice - #34221
Open
Qiming Bao (14H034160212) wants to merge 1 commit into
Open
Qiming Bao (14H034160212) wants to merge 1 commit into
Qiming Bao (14H034160212) wants to merge 1 commit into
Conversation
When a removed Cast's output feeds the same consumer more than once, e.g.
Where(condition, C, C), RemoveCastNodesChain and RemoveBackToBackCasts
removed and re-added the edge of the first matching input only, but
replaced every matching input def. The edge to the second input was left
behind and later failed in Graph::RemoveEdge ("Argument mismatch when
removing edge") or Graph::RemoveNode. Move each matching input edge.
Fixes microsoft#34192
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues for the fix preserving graph connectivity when Cast removal affects multiple inputs of the same consumer.
No open issues 🎉
What changed in this PR
Fixes #34192 by making PropagateCastOps preserve graph connectivity when a removed Cast feeds multiple inputs of the same consumer, such as Where(condition, C, C).
Changes:
- Rewires every matching consumer input edge in cast-chain removal and the multi-child back-to-back Cast removal path.
- Adds regression coverage for both removal paths, both propagation strategies, and levels 0–2.
| File | Description |
|---|---|
onnxruntime/test/optimizer/graph_transform_test.cc |
Adds repeated-consumer-input tests with and without another consumer of the FLOAT Cast. |
onnxruntime/core/optimizer/propagate_cast_ops.cc |
Updates both removal loops to move each affected edge and input definition together. |
Review effort: Balanced
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
PropagateCastOpsthrows when a Cast it removes feeds the same consumer more than once, as inWhere(condition, C, C).RemoveCastNodesChainand the multi-child branch ofRemoveBackToBackCastslooked up the consumer input withoptimizer_utils::IndexOfNodeInput, which returns the first match. They removed and re-added only that edge, whilestd::replacerewired every matching input def. The edge into the second input was left pointing at the removed Cast. Cleaning it up later then fails:Graph::RemoveEdge ... Argument mismatch when removing edge.Graph::RemoveNode ... Can't remove node ... as it still has output edges.Both loops now go over every input of the consumer that reads the removed output, and move each edge.
Motivation and Context
Fixes #34192.
Tests
New test
GraphTransformationTests.PropagateCastOpsTests_RepeatedConsumerInputbuildsX(fp16) -> Cast(float) -> Cast(fp16) -> C,Y = Where(true, C, C). A second variant gives the float Cast another consumer, which exercises the multi-child branch. Each variant runs with both strategies at levels 0 to 2 throughTestGraphTransformer. The test checks that the fp16 Cast is removed and that bothWheredata inputs read the graph input.main, the test fails with the two exceptions above.onnxruntime_test_all --gtest_filter='GraphTransformation*:*Optimizer*:*Fusion*:GraphUtils*:*PropagateCast*': 793 tests pass (Linux, CPU, Release).